Add additionalMounts for GitHub Action Output Mount by andar1an · Pull Request #219 · devcontainers/ci

andar1an · 2023-02-25T16:55:51Z

Beginning PR to add additional mounts required for GitHub Output in CI.

Recreating #187 work with a non-stale base.

WIP(DevContainerCliUpArgs - initial commit to add additional mounts for GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

There are a few deletions I did not make:

Lines 188, 195, and 200 in common/src/dev-container-cli.ts.
Lines 177 to 125 were formatted on save.

@stuartleeks update:
Fixes #180, #188, #195

…or GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

andar1an · 2023-02-25T16:57:57Z

I am unsure how to go about testing this, some direction here would be wonderful. Thank you!

stuartleeks · 2023-02-27T21:02:48Z

In terms of testing, this is a good nudge to add something to the docs - thank you!

I just ran a test and discovered that there are a couple of things that have snuck through that prevent running tests in a fork and have created #220.

Once that is merged, you can go to the Actions tab for your fork and you will see the various workflows listed:

Click on CI (branch) as shown in the image above, and then click on the Run workflow button:

This will pop up some options. Select your PR branch, enter a number in the PR number box, and untick the run full tests box (the last bit is important):

This should do a run for most of the GitHub action tests. The Azure DevOps tests require an environment to be set up and configured which can be triggered by maintainers against a PR.

andar1an · 2023-02-27T21:47:13Z

@stuartleeks should I add back those deleted lines? I am not sure if it was my ide, or if a merge happened within the time I had synced and pushed.

stuartleeks · 2023-02-28T06:58:57Z

@stephenandary - yes, those lines are needed

andar1an · 2023-02-28T11:38:22Z

@stuartleeks in my tests I am getting hit by some permission, env token, and 429 errors, can't get past the first block. May require you to approve the test workflow to have tests run with correct secrets. Also, does a test need to be made for mounts? I did not do that.

stuartleeks · 2023-02-28T11:59:38Z

@stephenandary - #220 has been merged now, so if you merge that into your PR branch you should be able to use the steps above to run the subset of GH tests that don't need credentials on your fork (i.e. without needing someone else to trigger a test run)

In terms of tests, yes I think it would be good to add a test showing that GITHUB_OUTPUT is working from a runCmd

…or GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

andar1an · 2023-02-28T12:30:58Z

@stuartleeks do you recall what the mount path should be from #187 (comment)?

…erUp ... again lol

…or GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

andar1an · 2023-03-01T15:29:56Z

@stuartleeks do you recall what the mount path should be from #187 (comment)?

@stuartleeks, just bubbling this up again, as it will save me time having to figure this out if you know. Unless this is not a need to know for runCmd test.

I also merged main, I was 1 commit behind which explains those little changes that were required.

stuartleeks · 2023-03-01T15:48:51Z

There was some discussion around the GITHUB_OUTPUT handling in #188 - is that what you mean?

andar1an · 2023-03-01T15:53:21Z

Yes, that is perfect! I forgot about that one! Mount is /mnt/github/output, thank you!

andar1an · 2023-03-01T17:20:56Z

@stuartleeks based on the approach you highlighted in #188, I am unsure what tests are appropriate to modify.

I have added mounts in common/__tests__, (this does not test /mnt/github/output). I noticed that there are also github-tests. Should I be adding a mount or environment variable into the devcontainer.jsons for the various scenarios there? By the description of the solution approach, it seems this should be automatic; I am not understanding how this is currently automatic and how only the common test will cover it.

stuartleeks · 2023-03-02T08:20:33Z

The tests in common\__tests__ are unit tests, whereas the contents under github-tests are used by the integration tests in CI.
The folder is slightly misnamed now as the content is used by both GitHub workflow tests and Azure Devops pipeline tests! The GitHub workflow to exercise them is in .github/workflows/ci_common.yml and the Azure DevOps pipeline to use them is in .azure-devops/azure-pipelines.yml

andar1an · 2023-03-02T13:03:27Z

Thanks @stuartleeks! I think where I am getting confused a bit is to whether a user will need to add the GITHUB_OUTPUT mount in the devcontainer.json to use env variable, or if there is a way that does not require user input. I am going to go down the road of adding the mount this morning, and hopefully, this becomes more clear during that exercise.

andar1an · 2023-03-02T13:54:21Z

@stuartleeks ,
To highlight where I am currently at, with your proposed approach from #188

mount the host's GITHUB_OUTPUT location to /mnt/github/output inside the dev container
set the GITHUB_OUTPUT environment variable in the container to /mnt/github/output

Currently, in integration tests, I am thinking of adding a mount along the lines of:

"mounts": [
		// Keep command history 
		"source=build-args-bashhistory,target=/home/vscode/commandhistory",
		// Mount to utilize GITHUB_OUTPUTS
		"source=/home/runner/work/${REPO}/${REPO}/.github/${GITHUB_ACTION}/${STEP_NAME}/output, target=/mnt/github/output"
],

I am not sure if this is the correct approach so trying to document in PR as I go. I imagine I would need to update the DOCKERFILE to reflect the GITHUB_OUTPUT env variable. I am hoping you know how I can do this without the env.variables as I don't think they will work here. Will a user also need to do both of these things?

Does this make sense to you?

stuartleeks · 2023-03-02T14:12:18Z

I imagined that the action code would determine the values for GITHUB_OUTPUT etc on the host and automatically add that as an additional mount when invoking the CLI, at the same time also setting the GITHUB_OUTPUT environment variable for the container to point to the mounted location. And then to do this for GITHUB_STATE etc.

It has occurred to me that anyone using this action and setting outputs/environment variables etc will be using the same approach that the action uses (which is now deprecated). We should make sure we get this change in soon to allow people chance to migrate to the new approach prior to the current approach being disabled. We're also seeing an quite a lot of failures in CI builds that seem to stem from the existing set-output approach not always working (I recently added some additional output to confirm this). I'm hoping that getting this change in will also improve the build reliability.

andar1an · 2023-03-02T14:22:55Z

@stuartleeks, to make sure I understand, I should not worry about runArgs and buildArgs tests, and just update the .github/workflows/ci_common.yml and .azure-devops/azure-pipelines.yml actions? Currently that is any action step that may contain:

echo "VERSION_MAJOR=${VERSION_MAJOR}"
echo "VERSION_MINOR=${VERSION_MINOR}"
echo "VERSION_PATCH=${VERSION_PATCH}"

…o prevent error TS2322: Type 'string' is not assignable to type 'string[]'

…t on wrong branch

…package.sh

…ange

…in build-package.sh

…ple say it fixes ambiguous redirect, they have no idea why

Fix node12 and set-output warnings

Update more actions for Nodejs12 and set-state warnings

stuartleeks · 2023-03-04T10:57:08Z

As discussed in one of the earlier comments, I pushed a couple of commits to add the mounts/env vars for GITHUB_OUTPUT and that resolved the errors you were seeing around quoting $GITHUB_OUTPUT in the script.

I also pushed the changes from #221 here, so I've closed that PR in favour of this one. I pushed this to the pr-219 branch in the upstream repo to kick of the tests and found a few more action upgrades that were needed to remove some more warnings around NodeJS versions, set-state etc. I've pushed those and this run is a clean run 🎉

Thanks for all your work on this @stephenandary !

andar1an · 2023-03-04T12:30:49Z

@stuartleeks this is so wonderful! Thank you for helping me learn. It would have taken me a while to figure out those additional changes and I'm grateful you could take this over the finish line. It was an enjoyable experience to work on, and thats all you! P.S. this was my first Open Source PR ever haha, going to save this one as a memento!

stuartleeks · 2023-03-06T08:47:06Z

@stephenandary - you are welcome. Thanks again for your work on this, and here's to many more OSS PRs 😁

chrmarti

LGTM!

@stuartleeks

@stuartleeks

WIP(DevContainerCliUpArgs - initial commit to add additional mounts f…

5b081c7

…or GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

andar1an requested review from a team and stuartleeks as code owners February 25, 2023 16:55

stuartleeks reviewed Feb 27, 2023

View reviewed changes

Comment thread common/src/dev-container-cli.ts

stuartleeks mentioned this pull request Feb 28, 2023

Handle GITHUB_OUTPUT #188

Closed

WIP(DevContainerCliUpArgs) - add back missing lines 188, 195, and 200

8bedd62

stephenandary added 2 commits February 28, 2023 07:08

WIP(DevContainerCliUpArgs - initial commit to add additional mounts f…

9cee4fc

…or GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

Merge branch 'cliupargs_additionalmounts'

78d12d2

andar1an commented Feb 28, 2023

View reviewed changes

Comment thread azdo-task/vss-extension.json

stephenandary added 4 commits February 28, 2023 07:39

WIP(DevContainerCliUpArgs) - add back missing env lines in devContain…

2907cc1

…erUp ... again lol

WIP(DevContainerCliUpArgs - initial commit to add additional mounts f…

27bbee2

…or GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

WIP(DevContainerCliUpArgs) - add back missing lines 188, 195, and 200

b7f72fa

Merge branch 'main' into cliupargs_additionalmounts

40874e3

WIP(cliupargs_additionalmounts) - add mounts test to config.ts

6a39278

stephenandary added 6 commits March 3, 2023 07:52

WIP(cliupargs_additionalmounts) - try adding split for mounts array t…

736535a

…o prevent error TS2322: Type 'string' is not assignable to type 'string[]'

WIP(cliupargs_additionalmounts) - revert last change, was running tes…

508f3c1

…t on wrong branch

WIP(cliupargs_additionalmounts) - revert mounts change in config.ts

59badc8

WIP(cliupargs_additionalmounts) - try setting GITHUB_OUTPUT mount in …

32918cc

…package.sh

WIP(cliupargs_additionalmounts) - revert last change and quotation ch…

7c3a510

…ange

WIP(cliupargs_additionalmounts) - remove quotes around GITHUB_OUTPUT …

735fdf7

…in build-package.sh

andar1an commented Mar 3, 2023

View reviewed changes

Comment thread azdo-task/scripts/build-package.sh Outdated

stephenandary and others added 7 commits March 3, 2023 08:47

WIP(cliupargs_additionalmounts) - put quotes aroung again as some peo…

1dc49ee

…ple say it fixes ambiguous redirect, they have no idea why

run build-local.sh

02ce049

Add env var/mount for GITHUB_OUTPUT

812197f

Update github-output test

98260a7

Add GITHUB_ENV and GITHUB_PATH handling

24559f1

Update to docker/setup-qemu-action@v2

1cb4f6c

Fix node12 and set-output warnings

Update docker actions

0bc1593

stuartleeks mentioned this pull request Mar 4, 2023

Bump actions/core to 1.10.0 #221

Closed

More action updates

149a922

Update more actions for Nodejs12 and set-state warnings

stuartleeks requested a review from chrmarti March 4, 2023 10:58

chrmarti approved these changes Mar 6, 2023

View reviewed changes

stuartleeks merged commit df4ab16 into devcontainers:main Mar 6, 2023

stuartleeks mentioned this pull request Mar 6, 2023

Update to actions/checkout@v3 #195

Closed

Sam13 mentioned this pull request Sep 11, 2025

GitHub event payload (GITHUB_EVENT_PATH) not mounted into devcontainer #406

Open

Conversation

andar1an commented Feb 25, 2023 • edited by stuartleeks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andar1an commented Feb 25, 2023

Uh oh!

Uh oh!

stuartleeks commented Feb 27, 2023

Uh oh!

andar1an commented Feb 27, 2023

Uh oh!

stuartleeks commented Feb 28, 2023

Uh oh!

andar1an commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartleeks commented Feb 28, 2023

Uh oh!

andar1an commented Feb 28, 2023

Uh oh!

Uh oh!

andar1an commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartleeks commented Mar 1, 2023

Uh oh!

andar1an commented Mar 1, 2023

Uh oh!

andar1an commented Mar 1, 2023

Uh oh!

stuartleeks commented Mar 2, 2023

Uh oh!

andar1an commented Mar 2, 2023

Uh oh!

andar1an commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartleeks commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andar1an commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stuartleeks commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andar1an commented Mar 4, 2023

Uh oh!

stuartleeks commented Mar 6, 2023

Uh oh!

chrmarti left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andar1an commented Feb 25, 2023 •

edited by stuartleeks

Loading

andar1an commented Feb 28, 2023 •

edited

Loading

andar1an commented Mar 1, 2023 •

edited

Loading

andar1an commented Mar 2, 2023 •

edited

Loading

stuartleeks commented Mar 2, 2023 •

edited

Loading

andar1an commented Mar 2, 2023 •

edited

Loading

stuartleeks commented Mar 4, 2023 •

edited

Loading